{183728233}: Add Lua push write by emelialei88 · Pull Request #6027 · bloomberg/comdb2

emelialei88 · 2026-06-15T16:43:41Z

No description provided.

roborivers

Cbuild submission: Success ✓.
Regression testing: 4/679 tests failed ⚠.

The first 10 failing tests are:
queuedb_rollover_noroll1_generated **quarantined**
cdb2jdbc
consumer_non_atomic_default_consumer_generated **quarantined**
fdb_compat
sc_downgrade [timeout] **quarantined**

dorinhogea · 2026-06-16T17:26:31Z

+    rc = lua_prepare_sql(sp, sql, &stmt);
+    if (rc == SQLITE_SCHEMA_PUSH_REMOTE_WRITE) {
+        rc = handle_fdb_push_write(sp->clnt, &err, 0, NULL, sql);
+        if (rc == -2) {


disable_fdb_push would run remtran, which we do not want; we can remove the retry here and simply return error (at this point push is available everywhere)

dorinhogea · 2026-06-16T17:29:52Z

        return -1;
    }
-    rc = cdb2_run_statement(hndl, clnt->sql);
+    rc = cdb2_run_statement(hndl, sql ? sql : clnt->sql);


we could pass clnt->sql as argument and always use sql here

roborivers

Cbuild submission: Error ⚠.
Regression testing: Success ✓.

The first 10 failing tests are:
ssl_san
consumer_non_atomic_default_consumer_generated **quarantined**
fdb_compat
ssl_set_cmd
ssl_prefer
ssl_dbname
sc_downgrade [timeout] **quarantined**
sc_timepart [timeout] **quarantined**

roborivers

Cbuild submission: Error ⚠.
Regression testing: Success ✓.

The first 10 failing tests are:
sc_resume_logicalsc_generated **quarantined**
sp_snapshot_generated
sc_parallel_logicalsc_generated
consumer_non_atomic_default_consumer_generated **quarantined**
fdb_push_lua
fdb_compat
sc_downgrade [timeout] **quarantined**

roborivers

Cbuild submission: Success ✓.
Regression testing: Success ✓.

The first 10 failing tests are:
sc_resume_logicalsc_generated **quarantined**
consumer_non_atomic_default_consumer_generated **quarantined**
fdb_push_lua
manual_partition
fdb_compat
sc_downgrade [timeout] **quarantined**

roborivers

Cbuild submission: Error ⚠.
Regression testing: Success ✓.

roborivers

Cbuild submission: Error ⚠.
Regression testing: Success ✓.

roborivers

Cbuild submission: Error ⚠.
Regression testing: Success ✓.

roborivers

Cbuild submission: Success ✓.
Regression testing: Success ✓.

roborivers

Cbuild submission: Success ✓.
Regression testing: Success ✓.

roborivers

Cbuild submission: Success ✓.
Regression testing: Success ✓.

The first 10 failing tests are:
sc_redo [failed with core dumped]
sc_resume_logicalsc_generated **quarantined**
consumer_non_atomic_default_consumer_generated **quarantined**
fdb_compat
sc_downgrade [timeout] **quarantined**

roborivers

Cbuild submission: Error ⚠.
Regression testing: 0/0 tests failed ⚠.

roborivers

Cbuild submission: Error ⚠.
Regression testing: 0/0 tests failed ⚠.

roborivers

Cbuild submission: Error ⚠.
Regression testing: 0/0 tests failed ⚠.

roborivers

Cbuild submission: Error ⚠.
Regression testing: 0/0 tests failed ⚠.

roborivers

Cbuild submission: Error ⚠.
Regression testing: Success ✓.

The first 10 failing tests are:
sirace [failed with core dumped]
sc_force [failed with core dumped]
rebuild_table_options [failed with core dumped]
odh_blobs [failed with core dumped]
verify_writes [failed with core dumped]
serial_randtest2 [failed with core dumped]
sicountbug [failed with core dumped]
incremental_backup [failed with core dumped]
maxtable
phys_rep_tiered

roborivers

Cbuild submission: Success ✓.
Regression testing: Success ✓.

The first 10 failing tests are:
sirace [failed with core dumped]
sc_force [failed with core dumped]
odh_blobs [failed with core dumped]
rebuild_table_options [failed with core dumped]
verify_writes [failed with core dumped]
serial_randtest2 [failed with core dumped]
sicountbug [failed with core dumped]
incremental_backup [failed with core dumped]
maxtable
phys_rep_tiered

roborivers

Cbuild submission: Success ✓.
Regression testing: Success ✓.

The first 10 failing tests are:
simpleauth_password [setup failed]
sc_resume_logicalsc_generated **quarantined**
reco-ddlk-sql **quarantined**
consumer_non_atomic_default_consumer_generated **quarantined**
fdb_compat
sc_downgrade [timeout] **quarantined**

roborivers

Cbuild submission: Success ✓.
Regression testing: Success ✓.

The first 10 failing tests are:
sqllogfill_reset_gen
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**
reco-ddlk-sql [timeout] **quarantined**

roborivers

Cbuild submission: Error ⚠.
Regression testing: Success ✓.

The first 10 failing tests are:
sc_resume_logicalsc_generated **quarantined**
ssl_san
consumer_non_atomic_default_consumer_generated **quarantined**
ssl_set_cmd
ssl_prefer
ssl_dbname
sc_downgrade [timeout] **quarantined**

roborivers

Cbuild submission: Error ⚠.
Regression testing: Success ✓.

The first 10 failing tests are:
sc_resume_logicalsc_generated **quarantined**
ssl_san
consumer_non_atomic_default_consumer_generated **quarantined**
manual_partition
ssl_set_cmd
ssl_prefer
ssl_dbname
skipscan **quarantined**
sc_downgrade [timeout] **quarantined**
reco-ddlk-sql [timeout] **quarantined**

roborivers

Cbuild submission: Success ✓.
Regression testing: Success ✓.

The first 10 failing tests are:
consumer_non_atomic_default_consumer_generated **quarantined**
commit_lsn_map
sc_downgrade [timeout] **quarantined**

roborivers

Cbuild submission: Success ✓.
Regression testing: Success ✓.

The first 10 failing tests are:
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**
reco-ddlk-sql [timeout] **quarantined**

dorinhogea · 2026-07-23T12:02:02Z

+
+    int rc;
+    if (dbstmt->fdb_push) {
+        struct errstat err;


initialize this to {0}

dorinhogea · 2026-07-23T12:04:31Z

        free(params);
    }

    clnt->fdb_push = push;


do we leak fdb_push here when we run multiple writes in same sp?

dorinhogea · 2026-07-23T12:06:13Z

+            lua_pushnil(lua);
+            lua_pushinteger(lua, sp->rc);
+        } else {
+            lua_pushinteger(lua, 0);


Claude flagged this as wrong stack: "After lua_remove(1) the stack is [sql]. On push-write success only lua_pushinteger(lua,0) is pushed, then return 2, so
Lua returns (sql_string, 0) instead of (nil, 0). Any caller doing local h, rc = db:exec("insert into remote...") gets
the SQL string (truthy) as the handle. Fix: add a leading lua_pushnil(lua) before the 0. The error branch is already
correct."

dorinhogea · 2026-07-23T12:09:45Z

+
+    struct param_data *param = &fdb_push_get_params(push)[position - 1];
+    if ((param->type == CLIENT_CSTR || param->type == CLIENT_BLOB) && param->len > 0)
+        free(param->u.p);


is param->len for a empty C string 0 or 1? if it is zero, do we leak a one byte here

dorinhogea · 2026-07-23T12:10:29Z

+        fdb_push_connector_t *push = sp->clnt->fdb_push;
+        int nparams = sqlite3_bind_parameter_count(stmt);
+        if (nparams > 0) {
+            fdb_push_set_params(push, calloc(nparams, sizeof(struct param_data)));


check calloc return for NULL

dorinhogea · 2026-07-23T13:04:04Z

+    int type;
+    if ((type = luabb_dbtype(lua, value)) > DBTYPES_MINTYPE) {
+        if (luabb_isnull(lua, value)) {
+            param->null = 1;


I think we need to do something like:

1959 + switch (type) { 1960 + case DBTYPES_INTEGER: param->type = CLIENT_INT; break; 1961 + case DBTYPES_REAL: param->type = CLIENT_REAL; break; 1962 + case DBTYPES_CSTRING: param->type = CLIENT_CSTR; break; 1963 + case DBTYPES_BLOB: param->type = CLIENT_BLOB; break; 1964 + case DBTYPES_DATETIME: param->type = CLIENT_DATETIME; break; 1965 + default: param->type = CLIENT_INT; break; 1966 + }

Reproducing test case:

# ============================================================ # Test 17: bind a typed NULL (a NULL read from a query column, # not a plain lua nil) -- reproduces typed-null bind # ============================================================ echo "Test 17: bind typed NULL from query column" >> $output $S - >> $output 2>&1 <<EOF create procedure test_typed_null version 'test17' { local function main() -- A NULL read back from a query comes through as a *typed* null -- (a dbtype userdata), unlike the plain lua nil used in Test 6. local sel = db:exec("SELECT NULL as x") local row = sel:fetch() local stmt, rc = db:prepare("INSERT INTO LOCAL_${a_remdbname}.t2(i, r, s) VALUES (@i, @r, @s)") if rc ~= 0 then return db:error(rc) end stmt:bind('i', 200) stmt:bind('r', 2.5) stmt:bind('s', row.x) stmt:exec() end }\$\$ exec procedure test_typed_null() EOF # Verify: row 200 must exist on the remote with s NULL $R "SELECT i, r, s FROM t2 WHERE i=200" >> $output 2>&1

dorinhogea

please see comments, thank you

emelialei88 · 2026-07-24T16:13:58Z

    if (!clnt->in_client_trans) {
        goto free;
    }


Follow-up (perf, non-blocking): SP push writes reconnect on every exec.

When the SP is not in an outer transaction (!clnt->in_client_trans), each push write closes its remote connection right after running (fdb_free_tran, db/fdb_push.c:820).

So this loop reconnects every time:

local stmt = db:prepare("INSERT INTO LOCAL_remdb.t VALUES (?, ?)") for i = 1, 10000 do stmt:bind(1, i); stmt:bind(2, 'row'..i); stmt:exec() -- reconnects each time end

Gotcha: db:begin() inside the SP does not help. It doesn't set in_client_trans. Only an outer client transaction reuses the handle:

begin exec procedure ... commit

Fix: keep the remote connection open across execs in one SP call. Free it once at SP teardown (reset_sp). Gate it so it never double-frees a dtran owned by an outer transaction. Semantics stay the same — each write is already auto-committed remotely. We just skip the reconnect.

emelialei88 · 2026-07-24T16:18:32Z

        if (p->nparams && p->params)
            dohsql_free_params(&p->nparams, &p->params,
                               p->nparams-1);


why did we pass p->nparams-1? If the number of params is 1 does it mean we don't free anything?

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

…m leak Two bugs in the Lua push-write path, both exercised by tests/fdb_push_lua.test: 1. Shared-connector collision. All prepared push writes routed bind/exec through the single clnt->fdb_push slot, so a second db:prepare clobbered the first statement's params/remotedb. Each dbstmt now owns its connector: db:prepare detaches it from clnt->fdb_push onto dbstmt->push_conn, bind targets that connector directly (never clnt->fdb_push, which a bind-error longjmp could leave dangling), exec installs/restores it around the write, and donate_stmt frees it. Covered by Test 18 (two concurrently-prepared push statements) — fails on the old code with a bogus out-of-range bind. 2. Prepare-time param leak. When an SP is invoked with bind args, fdb_push_write_setup clones those params into the connector; db_prepare then overwrote that array with a fresh one for the statement's own placeholders, leaking the clone. db_prepare now calls the new fdb_push_clear_params() before installing its array. Covered by Test 19 (SP invoked with a bind arg + push prepare). Also downgrade two LOGMSG_WARN in fdb_push_write_setup to LOGMSG_DEBUG: the non-push fallback is normal control flow, not a warning. Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

roborivers

Cbuild submission: Error ⚠.
Regression testing: Success ✓.

The first 10 failing tests are:
reco-ddlk-sql **quarantined**
ssl_san
consumer_non_atomic_default_consumer_generated **quarantined**
ssl_set_cmd
ssl_prefer
ssl_dbname
sc_downgrade [timeout] **quarantined**

roborivers

Cbuild submission: Success ✓.
Regression testing: Success ✓.

The first 10 failing tests are:
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**

emelialei88 force-pushed the feat/lua-push branch 2 times, most recently from 1fe3177 to d7dd67d Compare June 15, 2026 16:48

roborivers suggested changes Jun 15, 2026

View reviewed changes

dorinhogea reviewed Jun 16, 2026

View reviewed changes

Comment thread lua/sp.c

emelialei88 force-pushed the feat/lua-push branch from d7dd67d to 2372267 Compare June 16, 2026 20:44

This comment was marked as outdated.

Sign in to view

emelialei88 force-pushed the feat/lua-push branch from 2372267 to ea9dda9 Compare June 18, 2026 19:59

roborivers suggested changes Jun 18, 2026

View reviewed changes

emelialei88 force-pushed the feat/lua-push branch from ea9dda9 to 5bcbc89 Compare June 24, 2026 18:37

roborivers suggested changes Jun 24, 2026

View reviewed changes

roborivers approved these changes Jun 25, 2026

View reviewed changes

emelialei88 force-pushed the feat/lua-push branch 3 times, most recently from 28f01d5 to ec68fcd Compare June 25, 2026 19:57

roborivers suggested changes Jun 25, 2026

View reviewed changes

emelialei88 force-pushed the feat/lua-push branch from ec68fcd to 6c09249 Compare June 25, 2026 20:47

roborivers suggested changes Jun 25, 2026

View reviewed changes

roborivers approved these changes Jun 26, 2026

View reviewed changes

emelialei88 force-pushed the feat/lua-push branch from 6c09249 to 4c0f998 Compare June 29, 2026 22:03

roborivers suggested changes Jun 29, 2026

View reviewed changes

roborivers suggested changes Jun 30, 2026

View reviewed changes

emelialei88 force-pushed the feat/lua-push branch 2 times, most recently from df2c0cb to f69789d Compare June 30, 2026 16:21

roborivers suggested changes Jun 30, 2026

View reviewed changes

roborivers approved these changes Jun 30, 2026

View reviewed changes

emelialei88 force-pushed the feat/lua-push branch from f69789d to 5c0ffe8 Compare July 1, 2026 12:38

roborivers approved these changes Jul 1, 2026

View reviewed changes

roborivers suggested changes Jul 1, 2026

View reviewed changes

emelialei88 force-pushed the feat/lua-push branch from 5c0ffe8 to 8b6b1f1 Compare July 1, 2026 15:57

roborivers suggested changes Jul 1, 2026

View reviewed changes

emelialei88 force-pushed the feat/lua-push branch from 8b6b1f1 to b7b83f0 Compare July 1, 2026 18:23

roborivers approved these changes Jul 1, 2026

View reviewed changes

emelialei88 changed the title ~~{183728233}: Add Lua push~~ {183728233}: Add Lua push write Jul 2, 2026

emelialei88 force-pushed the feat/lua-push branch from b7b83f0 to e9f9670 Compare July 7, 2026 16:44

roborivers approved these changes Jul 7, 2026

View reviewed changes

emelialei88 marked this pull request as ready for review July 20, 2026 14:24

dorinhogea reviewed Jul 23, 2026

View reviewed changes

dorinhogea requested changes Jul 23, 2026

View reviewed changes

emelialei88 commented Jul 24, 2026

View reviewed changes

emelialei88 added 2 commits July 24, 2026 12:49

support lua push

89220c2

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

solve PR feedback

995b353

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

emelialei88 force-pushed the feat/lua-push branch from e9f9670 to 1dc1952 Compare July 24, 2026 16:49

emelialei88 force-pushed the feat/lua-push branch from 1dc1952 to c537a5a Compare July 24, 2026 17:06

roborivers suggested changes Jul 24, 2026

View reviewed changes

roborivers approved these changes Jul 24, 2026

View reviewed changes

Uh oh!

Conversation

emelialei88 commented Jun 15, 2026

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

roborivers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dorinhogea Jul 23, 2026 •

edited by emelialei88

Loading